Introduce tracer API within isolated module. #3043

raphw · 2023-03-01T20:54:42Z

What does this PR do?

This PR introduces a minimal, zero-dependency Tracer API. This API is a superset of the current Tracer API which is part of the core module. The API was identified to be the minimal subset of API that is required by the majority of plugins. As a result, plugins can be moved away from depending upon the apm-agent-core module but only rely on this API module. This decouples the plugins from the agents implementation what allows for a cleaner modularization, and it opens for the reuse of the Elastic agent plugins outside of the current implementation of the agent.

A large fraction of the changes of this PR are IDE generated. The changes were generated by creating necessary interfaces, and by later pulling members upwards. At the same time, javadoc was moved upwards the interface hierarchy.

Checklist

This is something else
- I have updated CHANGELOG.asciidoc

github-actions · 2023-03-01T20:54:59Z

👋 @raphw Thanks a lot for your contribution!

It may take some time before we review a PR, so even if you don’t see activity for some time, it does not mean that we have forgotten about it.

Every once in a while we go through a process of prioritization, after which we are focussing on the tasks that were planned for the upcoming milestone. The prioritization status is typically reflected through the PR labels. It could be pending triage, a candidate for a future milestone, or have a target milestone set to it.

apmmachine · 2023-03-01T21:02:00Z

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

Start Time: 2023-03-09T14:39:14.640+0000
Duration: 54 min 11 sec

Test stats 🧪

Test	Results
Failed	0
Passed	3559
Skipped	121
Total	3680

💚 Flaky test report

Tests succeeded.

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

/test : Re-trigger the build.
run benchmark tests : Run the benchmark tests.
run jdk compatibility tests : Run the JDK Compatibility tests.
run integration tests : Run the Agent Integration tests.
run end-to-end tests : Run the APM-ITs.
run windows tests : Build & tests on windows.
run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

SylvainJuge

This looks great, I only have a few comments/questions.

I think the biggest question here is "do we want to use this API+Impl naming style", because this is not something that we have done so far, so it's a bit new and might need a bit of thought before committing to it.

I think the name duplication is fine as this is an "internal API", there is little ambiguity between the interfaces and their implementations and there is always a 1:1 relationship between them. For very simple types like Scope or Outcome we can even avoid creating types outside of the tracer-api.

CHANGELOG.asciidoc

apm-agent-tracer/src/main/java/co/elastic/apm/agent/tracer/Tracer.java

apm-agent-core/src/main/java/co/elastic/apm/agent/impl/ElasticApmTracer.java

apm-agent-core/src/main/java/co/elastic/apm/agent/impl/GlobalTracer.java

apm-agent-core/src/main/java/co/elastic/apm/agent/impl/NoopTracer.java

apm-agent-core/src/main/java/co/elastic/apm/agent/impl/transaction/Outcome.java

SylvainJuge · 2023-03-07T15:08:58Z

apm-agent-tracer/src/main/java/co/elastic/apm/agent/tracer/Tracer.java

+    @Nullable
+    <T extends Tracer> T probe(Class<T> type);
+
+    <T extends Tracer> T require(Class<T> type);


[question] I'm guessing the use-case for those methods might come from next steps in this refactoring, but do we need to have this kind of cast directly available in the Tracer API ?

This could be added in a next step, but I wanted to add it as part of the minimal API. The idea is to allow for casting the internal tracer of a global tracer to a specific implementation without the global tracer needing to know about a specific implementation.

This way it will still be possible to receive the ElasticApmTracer, or any other specific tracer implementation from a global tracer.

SylvainJuge · 2023-03-07T15:13:07Z

...lugins/apm-api-plugin/src/test/java/co/elastic/apm/api/ElasticApmApiInstrumentationTest.java

@@ -148,13 +147,13 @@ void testAutomaticAndManualTransactions() {
    void testGetId_distributedTracingEnabled() {

        co.elastic.apm.agent.impl.transaction.Transaction transaction = tracer.startRootTransaction(null).withType(Transaction.TYPE_REQUEST);
-        try (Scope scope = transaction.activateInScope()) {
+        try (co.elastic.apm.agent.tracer.Scope scope = transaction.activateInScope()) {


[minor] If I understand this correctly, in this case we can just change the original import and not use the FQN, and we will do the same when modifying all the plugins to use the newly created tracer API.

apm-agent-api does not currently depend on the new tracer module, so I wanted to avoid the additional refactoring in the first round.

raphw · 2023-03-07T22:57:01Z

As for the naming convention: I think it is a good way to evolve the code base as it does not leave any doubt about what APIs are migrated. At some late point, I think it is fair to remove all current interfaces as they are made redundant by the API module. But I would leave this last refactoring to one of the last stages.

SylvainJuge · 2023-03-08T10:30:16Z

You were faster to push than me, I only have a few tests to add here :-).
I will approve this PR so it can run on CI now, which is a requirement for external contributions.

SylvainJuge

Approve to allow running on CI.

SylvainJuge · 2023-03-08T10:40:48Z

As for the naming convention: I think it is a good way to evolve the code base as it does not leave any doubt about what APIs are migrated. At some late point, I think it is fair to remove all current interfaces as they are made redundant by the API module. But I would leave this last refactoring to one of the last stages.

So, if I understand this correctly:

the usage of duplicating names by adding interfaces is just an intermediate step and not a desired final state.
the short-term benefit for now is that it helps to migrate in steps and mostly require changing imports for now (which is trivial to review and has no impact).
Later, implementation of those interfaces will be moved to the tracer-api, replacing the interfaces. As a result there won't be duplicated class names anymore.

raphw · 2023-03-08T11:19:38Z

Some of the current interfaces contain methods that are only required by internal components, but not by the plugins itself. It would make sense to make the distinction clear, for example by naming it ElasticTransaction vs Transaction, similar to the Tracer API compared to the already existing ElasticApmTracer. I would expect that use of the latter would only be required from within apm-agent-core itself in a final migration, when this dependency is removed from the internal plugins.

raphw · 2023-03-08T12:13:28Z

The failing unit test struggles with Caused by: java.lang.ClassNotFoundException: io.vertx.core.impl.logging.LoggerFactory. I do not see that this can be related to these changes. Is this a known issue?

SylvainJuge · 2023-03-08T13:08:53Z

The failing unit test struggles with Caused by: java.lang.ClassNotFoundException: io.vertx.core.impl.logging.LoggerFactory. I do not see that this can be related to these changes. Is this a known issue?

Yes, this was fixed with #3044.
Updating this PR from main branch should make those errors go away.

Co-authored-by: SylvainJuge <[email protected]>

raphw · 2023-03-08T20:31:23Z

Alright, I rebased upon main, let's give it another try. Should I squash the commits or are you happy to keep the change set the way it is?

SylvainJuge · 2023-03-09T11:10:10Z

No need to squash the commits nor rewrite the history, we squash when merging, so it's easier to revert if needed and keeps the main history simple and clean. Personally, I usually just merge main to update dev branches without rebasing.

SylvainJuge · 2023-03-09T11:10:24Z

@elasticmachine run elasticsearch-ci/docs

github-actions bot added agent-java community Issues and PRs created by the community triage labels Mar 1, 2023

raphw force-pushed the tracer-api branch from 54e5f04 to b6a92a2 Compare March 1, 2023 20:58

raphw force-pushed the tracer-api branch 2 times, most recently from 3314cbc to 652b540 Compare March 1, 2023 21:10

AlexanderWert requested a review from SylvainJuge March 6, 2023 09:46

SylvainJuge removed the triage label Mar 7, 2023

SylvainJuge reviewed Mar 7, 2023

View reviewed changes

SylvainJuge approved these changes Mar 8, 2023

View reviewed changes

raphw force-pushed the tracer-api branch from f46d4dc to ba29d89 Compare March 8, 2023 11:37

raphw and others added 6 commits March 8, 2023 21:30

Introduce minimal, zero-dependency tracer API within isolated module.

ef1e796

Update CHANGELOG.asciidoc

b11e51f

Co-authored-by: SylvainJuge <[email protected]>

Replace Outcome interface with outcome enumeration.

0474ef3

Throw exception on asking for configuration that is not available.

bfb6e31

test non-null configuration

bfb4e30

Correct import symbols for open tracing bridge test.

ab6aa56

raphw force-pushed the tracer-api branch from ba29d89 to ab6aa56 Compare March 8, 2023 20:30

raphw mentioned this pull request Mar 9, 2023

Favour Tracer API above ElasticApmTracer API in plugins and add additional seperation #3030

Open

jackshirazi approved these changes Mar 10, 2023

View reviewed changes

jackshirazi merged commit 05efbe9 into elastic:main Mar 10, 2023

SylvainJuge mentioned this pull request Apr 12, 2023

[release] update 1.x branch #3107

Closed

19 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce tracer API within isolated module. #3043

Introduce tracer API within isolated module. #3043

raphw commented Mar 1, 2023 •

edited by SylvainJuge

Loading

github-actions bot commented Mar 1, 2023

apmmachine commented Mar 1, 2023 •

edited

Loading

Build stats

Test stats 🧪

SylvainJuge left a comment

SylvainJuge Mar 7, 2023

raphw Mar 7, 2023

SylvainJuge Mar 7, 2023

raphw Mar 7, 2023

raphw commented Mar 7, 2023

SylvainJuge commented Mar 8, 2023

SylvainJuge left a comment

SylvainJuge commented Mar 8, 2023

raphw commented Mar 8, 2023

raphw commented Mar 8, 2023

SylvainJuge commented Mar 8, 2023

raphw commented Mar 8, 2023

SylvainJuge commented Mar 9, 2023

SylvainJuge commented Mar 9, 2023

Introduce tracer API within isolated module. #3043

Introduce tracer API within isolated module. #3043

Conversation

raphw commented Mar 1, 2023 • edited by SylvainJuge Loading

What does this PR do?

Checklist

github-actions bot commented Mar 1, 2023

apmmachine commented Mar 1, 2023 • edited Loading

💚 Build Succeeded

Build stats

Test stats 🧪

💚 Flaky test report

🤖 GitHub comments

SylvainJuge left a comment

Choose a reason for hiding this comment

SylvainJuge Mar 7, 2023

Choose a reason for hiding this comment

raphw Mar 7, 2023

Choose a reason for hiding this comment

SylvainJuge Mar 7, 2023

Choose a reason for hiding this comment

raphw Mar 7, 2023

Choose a reason for hiding this comment

raphw commented Mar 7, 2023

SylvainJuge commented Mar 8, 2023

SylvainJuge left a comment

Choose a reason for hiding this comment

SylvainJuge commented Mar 8, 2023

raphw commented Mar 8, 2023

raphw commented Mar 8, 2023

SylvainJuge commented Mar 8, 2023

raphw commented Mar 8, 2023

SylvainJuge commented Mar 9, 2023

SylvainJuge commented Mar 9, 2023

raphw commented Mar 1, 2023 •

edited by SylvainJuge

Loading

apmmachine commented Mar 1, 2023 •

edited

Loading